Skip to content

feat(miner): roll the SqliteDriver seam onto the non-transactional local stores#7541

Closed
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:feat/sqlite-driver-seam-rollout-7282
Closed

feat(miner): roll the SqliteDriver seam onto the non-transactional local stores#7541
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:feat/sqlite-driver-seam-rollout-7282

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What

Continues the #7175 SqliteDriver seam rollout (#7282): migrates the genuinely-non-transactional AMS local stores onto the same additive, still-synchronous openLocalStoreAdapter / driver.query pattern already proven by run-state.js (#7194) and policy-doc-cache.js.

Migrated (4):

  • lib/contribution-profile-cache.js
  • lib/policy-verdict-cache.js
  • lib/prediction-ledger.js
  • lib/replay-snapshot.js

For each: the store is opened via openLocalStoreAdapter(resolvedPath) (returning { db, driver }), and all CRUD (SELECT/INSERT/UPDATE/DELETE) now routes through driver.query(sql, params). Schema creation, migrations, retention, and purge stay on the underlying db handle exactly as before. The public store APIs remain synchronous and unchanged — this is purely additive, mirroring the reference stores.

Verified out of scope (left unmigrated, on purpose)

Per the deliverable's own "verify each individually for hidden transaction/RETURNING usage" caveat, two of the six listed stores are not actually non-transactional and belong to the separate runOn(client) interactive-transaction design the issue tracks in its next checkbox:

  • ranked-candidates.jsreplaceAll is a genuine BEGIN IMMEDIATE → DELETE + multi-INSERT → COMMIT/ROLLBACK atomic block, which the current driver.query seam can't express.
  • worktree-allocator.jsacquire is an interactive BEGIN IMMEDIATE read-then-conditional-write transaction, and releaseByAttempt uses UPDATE … RETURNING …, which store-db-adapter.js's own doc explicitly flags as not-yet-safe to route through the seam.

Migrating either would silently break an atomic path, so both are correctly deferred.

No behavior change

The SQL, parameters, return shapes, and synchronous API of every migrated store are byte-identical — only the handle CRUD goes through the seam instead of a raw DatabaseSync prepared statement. Proof: all six stores' existing test suites pass completely unmodified (90/90), including the two unmigrated ones.

Validation

  • npm run typecheck — clean (one pre-existing unrelated discover-cli.ts error only).
  • All six store test files pass unchanged: miner-contribution-profile-cache, miner-policy-verdict-cache, miner-prediction-ledger, miner-replay-snapshot, miner-ranked-candidates, miner-worktree-allocator-collisions — 90/90.
  • Coverage: every changed CRUD-routing line in the 4 migrated files is covered by those existing tests (the only uncovered lines in the report are pre-existing untested paths I did not touch — e.g. prediction-ledger's corrupted-row throw and default-singleton wrappers).
  • npm run test:miner-pack passes; git diff --check clean.

Closes #7282

@shin-core
shin-core requested a review from JSONbored as a code owner July 20, 2026 21:13
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 20, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent did not find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.90%. Comparing base (37e7f86) to head (2379d62).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7541      +/-   ##
==========================================
- Coverage   88.47%   79.90%   -8.58%     
==========================================
  Files         720       88     -632     
  Lines       75733    23202   -52531     
  Branches    22547     3700   -18847     
==========================================
- Hits        67008    18539   -48469     
+ Misses       7679     4479    -3200     
+ Partials     1046      184     -862     
Flag Coverage Δ
shard-1 32.40% <51.85%> (-0.64%) ⬇️
shard-2 75.60% <77.77%> (+38.74%) ⬆️
shard-3 36.40% <70.37%> (+10.73%) ⬆️
shard-4 23.20% <22.22%> (-15.69%) ⬇️
shard-5 4.09% <74.07%> (-30.12%) ⬇️
shard-6 7.60% <0.00%> (-28.63%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...s/loopover-miner/lib/contribution-profile-cache.ts 100.00% <100.00%> (ø)
...ackages/loopover-miner/lib/policy-verdict-cache.ts 100.00% <100.00%> (ø)
packages/loopover-miner/lib/prediction-ledger.ts 80.88% <100.00%> (+0.28%) ⬆️
packages/loopover-miner/lib/replay-snapshot.ts 100.00% <100.00%> (ø)

... and 632 files with indirect coverage changes

@loopover-orb loopover-orb Bot added gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. labels Jul 20, 2026
@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-20 21:24:38 UTC

12 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Fix Blockers

Review summary
This PR mechanically re-plumbs four already-synchronous local SQLite stores (prediction-ledger, policy-verdict-cache, contribution-profile-cache, replay-snapshot) from raw `db.prepare().run()/.get()/.all()` calls onto the already-proven `openLocalStoreAdapter` + `driver.query(sql, params)` seam, exactly mirroring the reference implementation in run-state.js. SQL text, bind-parameter order, and return shapes are preserved 1:1 across every migrated call site (e.g. prediction-ledger.ts:238-263, replay-snapshot.ts:222-260), and schema creation/migrations/retention/purge correctly stay on the raw `db` handle as the PR description claims. The two genuinely transactional stores (ranked-candidates.js, worktree-allocator.js) are correctly left out of scope with a clear technical justification (BEGIN IMMEDIATE / RETURNING usage the seam can't express).

Nits — 6 non-blocking
  • CI shows `validate`, `validate-code`, and `validate-tests-merge` as FAILED on this commit — worth checking what's failing there before merge, since it isn't visible from the diff alone.
  • No new tests are added for this refactor (0 test-path lines per the coverage brief); that's consistent with the run-state.js precedent (a pure handle-level swap covered by each store's existing CRUD test suite), but worth confirming those existing suites actually exercise every migrated call site (e.g. contribution-profile-cache.ts:98-124's stale/corrupted-row branches) rather than just the happy path.
  • `packages/loopover-miner/lib/replay-snapshot.d.ts` and the other `.d.ts` files gained doc-comment updates but weren't shown in full in this diff — confirm they stay in sync with the `.ts` source they're generated from.
  • If CI's `validate`/`validate-code` failures are unrelated to this change (e.g. a flaky/broken base), link the failing job output in the PR so reviewers don't have to guess.
  • Consider a one-line changelog/PR-description note confirming which of the 6 originally-scoped stores are done vs. deferred, since this PR completes 4 of 6 and the remaining 2 need a different `runOn(client)` design — helps track the parent issue's remaining scope.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

CI checks failing

  • validate
  • validate-tests-merge
  • validate-code
Flagged checks (non-blocking)
  • Contributor trust — Contributor flagged for review

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7282
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 72 registered-repo PR(s), 47 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 72 PR(s), 0 issue(s).
Improvement ℹ️ None detected risk: low · value: none · LLM: moderate
Linked issue satisfaction

Partially addressed
The PR migrates 4 of the 6 stores listed in the issue's deliverable (contribution-profile-cache, prediction-ledger, replay-snapshot, plus a policy-verdict-cache referenced in the description but not shown in the diff), explicitly deferring ranked-candidates.js and worktree-allocator.js with justified transaction/RETURNING concerns per the issue's own caveat, but does not touch the other two delive

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
  • Official Gittensor activity: 72 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-tests-merge, validate-code)). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(miner): finish rolling the SqliteDriver seam onto AMS's remaining local stores

1 participant